@bitwalker Following up on #18 (comment)

I've pushed a commit that expands our type system with signed and unsigned equivalents, all of the standard operators (comparison, arithmetic, etc.) will be compiled based on the semantics of the types involved. I still have to work out what to do about signed integers in general - our only native option in MASM for signed integers are field elements, the support for u32 and u64 is only useful for unsigned ops, as you'd expect. That probably means that we either need to implement our own primitives for representing, say, signed 32-bit integers, or promote all signed 32-bit integers to field elements, but that comes with its own complexities. I suspect for now we'll need some combination of the two, i.e. using field elements for signed integers, with some additional generated code around ops involving them to protect the range of the type in question. I think we can probably kick this can down the road a bit though.

Sounds reasonable. I've tried to implement div_s/u and hit a roadblock. Since Wasm only has i32 and i64 types, how the sign is interpreted is up to the operation itself (see div_s vs. div_u). I cannot figure out how to translate signed/unsigned integer op to IR using only Type variants. I think we might need an interpret op (with a better name) that takes a value and returns the same value but with a given target type. So, it can be called before div_s/u to interpret the input values as signed/unsigned.
OTOH, for the rest of the ops that don't have signed/unsigned variants, the use of two’s complement for the signed interpretation means that they behave the same regardless of signedness. So if we use Type::I32 for the Wasm i32 value in 'add' op, it would not convey the "signedness", i.e., that we should treat a value as a signed integer. So, we end up where for the add op, i32 means nothing with regards to signedness, but for div op, it does mean that the value is signed.

Since Wasm only has i32 and i64 types, how the sign is interpreted is up to the operation itself (see div_s vs. div_u). I cannot figure out how to translate signed/unsigned integer op to IR using only Type variants. I think we might need an interpret op (with a better name) that takes a value and returns the same value but with a given target type.

Let's use cast for this for now, it basically does exactly this. We can think on whether we want more specific operations for this as we get further along, but for now this should unblock you.

OTOH, for the rest of the ops that don't have signed/unsigned variants, the use of two’s complement for the signed interpretation means that they behave the same regardless of signedness.

That's only true for addition/subtraction/multiplication AFAIK, most everything else is going to need to know how to interpret the binary representation in order to have the correct semantics, but perhaps those are precisely the lines along which the distinction is drawn in Wasm (in terms of which ops have _s/_u variants, and which don't).

In any case, I think the implication is that unless an op in Wasm distinguishes signed vs unsigned, we must assume that the semantics are signed - that is after all the purpose of the two's complement representation. Since Wasm uses dedicated ops to interpret a two's complement binary encoding as an plain unsigned encoding when desired, I think the equivalent approach in our IR would be to cast to Type::U32, and then call the appropriate op, in order to get unsigned semantics. Presumably, unsigned ops are only ever used on values of unsigned type, and vice versa, so I don't think it would actually be necessary to cast back to Type::I32 afterwards - i.e. the use of a _s/_u op has a type-narrowing effect.

While we could take a similar tack as Wasm, preferring to represent integers in the IR as signed by default, with dedicated unsigned ops; Miden is a bit all over the place with its instruction set: field elements are signed, but have a restricted set of operations, while its u32 instruction set is purely unsigned, with a broader set of operations. We can certainly emulate signed 32-bit integers with what Miden provides, but we're not going to get much out of the box - we'll have to implement special handling for basically all ops on signed values, e.g. not just comparisons, right shift, division; but also simple things like add, since it will be up to us to handle signed overflow. I'll do this on an ad-hoc basis as specific signed ops are needed.

For now at least, let's go with the cast approach, and see where that gets us. If that ends up not working well for other reasons, we can revisit this.

Since Wasm only has i32 and i64 types, how the sign is interpreted is up to the operation itself (see div_s vs. div_u). I cannot figure out how to translate signed/unsigned integer op to IR using only Type variants. I think we might need an interpret op (with a better name) that takes a value and returns the same value but with a given target type.

Let's use cast for this for now, it basically does exactly this. We can think on whether we want more specific operations for this as we get further along, but for now this should unblock you.

My first thought was to use cast but I was turned away from it by the strict wording of OpCode::Cast description: "It is not valid to perform a cast on any value other than a field element". I should've checked the InstBuilder::cast implementation.

OTOH, for the rest of the ops that don't have signed/unsigned variants, the use of two’s complement for the signed interpretation means that they behave the same regardless of signedness.

That's only true for addition/subtraction/multiplication AFAIK, most everything else is going to need to know how to interpret the binary representation in order to have the correct semantics, but perhaps those are precisely the lines along which the distinction is drawn in Wasm (in terms of which ops have _s/_u variants, and which don't).

In any case, I think the implication is that unless an op in Wasm distinguishes signed vs unsigned, we must assume that the semantics are signed - that is after all the purpose of the two's complement representation. Since Wasm uses dedicated ops to interpret a two's complement binary encoding as an plain unsigned encoding when desired, I think the equivalent approach in our IR would be to cast to Type::U32, and then call the appropriate op, in order to get unsigned semantics. Presumably, unsigned ops are only ever used on values of unsigned type, and vice versa, so I don't think it would actually be necessary to cast back to Type::I32 afterwards - i.e. the use of a _s/_u op has a type-narrowing effect.

While we could take a similar tack as Wasm, preferring to represent integers in the IR as signed by default, with dedicated unsigned ops; Miden is a bit all over the place with its instruction set: field elements are signed, but have a restricted set of operations, while its u32 instruction set is purely unsigned, with a broader set of operations. We can certainly emulate signed 32-bit integers with what Miden provides, but we're not going to get much out of the box - we'll have to implement special handling for basically all ops on signed values, e.g. not just comparisons, right shift, division; but also simple things like add, since it will be up to us to handle signed overflow. I'll do this on an ad-hoc basis as specific signed ops are needed.

For now at least, let's go with the cast approach, and see where that gets us. If that ends up not working well for other reasons, we can revisit this.

Thank you for the detailed explanation. It clears up a lot of things for me. I'll go with the cast to Type::U32 for *_u ops.

My first thought was to use cast but I was turned away from it by the strict wording of OpCode::Cast description: "It is not valid to perform a cast on any value other than a field element". I should've checked the InstBuilder::cast implementation.

Yeah that documentation needs clarification now that what I'm anticipating for it's usage has been dialed in a bit better. The primary thing I was trying to convey with that is that cast is not intended to support casting values of aggregate type (structs, arrays), or other large values. It is basically intended to be the equivalent of LLVM's bitcast instruction - in short, it is only intended to support casting between integral types that fit in a single field element, as well as pointer-to-pointer casts.

It may turn out that definition is too broad for cast, and perhaps having more specific casts with dedicated instructions is better, which we do actually have in some form with the zext, sext, and trunc instructions, which are more precise integer conversion instructions, but they don't really represent the notion of signed/unsigned conversion of the same bit size. It may make the most sense to rename cast to ptrcast, and reserve it for pointer casts only, and then add as_signed/as_unsigned instructions that specifically handle reinterpreting a signed integer as unsigned and vice versa. We'll need all three kinds of conversions in any case, it really is just going to boil down to how we expose them in the IR. For now though, I think you can just use cast, and we can perhaps discuss the overall approach in next week's compiler team meeting.

@bitwalker I pushed the Wasm globals implementation. global.get is translated into InstBuilder::load_symbol, and global.set is translated into InstBuilder::store using the address from InstBuilder::symbol_addr

https://github.com/greenhat/miden-ir/blob/c6dc663c2d42c37df49544333f72edeaddcff71c/frontend-wasm/src/code_translator.rs#L75-L90

I pushed the Wasm globals implementation. global.get is translated into InstBuilder::load_symbol, and global.set is translated into InstBuilder::store using the address from InstBuilder::symbol_addr

Perfect, that's what I was anticipating, just wasn't 100% sure if Wasm also had the equivalent of relative addressing and operations which use it, sounds like it's much more straightforward, which keeps things nice and simple.

@bitwalker I'm looking at the data segments in the spec, and it seems like we need to be able to load a byte array into the memory at a predefined address before starting the execution. There could be multiple data segments, each with its offset and size.

Here is how it looks in the wild. The following Rust example:

#[inline(never)]
#[no_mangle]
pub fn sum_arr(arr: &[u32]) -> u32 {
    arr.iter().sum()
}

#[no_mangle]
pub extern "C" fn __main() -> u32 {
    sum_arr(&[1, 2, 3, 4, 5]) + sum_arr(&[6, 7, 8, 9, 10])
}

Compiles into the following Wasm:

(module
    (type (;0;) (func (param i32 i32) (result i32)))
    (type (;1;) (func (result i32)))
    (func $sum_arr (;0;) (type 0) (param i32 i32) (result i32)
    (local i32)
    i32.const 0
    local.set 2
    block ;; label = @1
        local.get 1
        i32.eqz
        br_if 0 (;@1;)
        loop ;; label = @2
        local.get 0
        i32.load
        local.get 2
        i32.add
        local.set 2
        local.get 0
        i32.const 4
        i32.add
        local.set 0
        local.get 1
        i32.const -1
        i32.add
        local.tee 1
        br_if 0 (;@2;)
        end
    end
    local.get 2
    )
    (func $__main (;1;) (type 1) (result i32)
    i32.const 1048576
    i32.const 5
    call $sum_arr
    i32.const 1048596
    i32.const 5
    call $sum_arr
    i32.add
    )
    (memory (;0;) 17)
    (global $__stack_pointer (;0;) (mut i32) i32.const 1048576)
    (global (;1;) i32 i32.const 1048616)
    (global (;2;) i32 i32.const 1048624)
    (export "memory" (memory 0))
    (export "sum_arr" (func $sum_arr))
    (export "__main" (func $__main))
    (export "__data_end" (global 1))
    (export "__heap_base" (global 2))
    (data $.rodata (;0;) (i32.const 1048576) "\01\00\00\00\02\00\00\00\03\00\00\00\04\00\00\00\05\00\00\00\06\00\00\00\07\00\00\00\08\00\00\00\09\00\00\00\0a\00\00\00")
)

A data segment at the end of the module should be loaded into the memory at the address 1048576 (0x100000). There can be multiple data segments, each with its offset and size, but since the v1 spec doesn't support multiple memories, the memory index (;0;) should always be 0.

I'm looking at the data segments in the spec, and it seems like we need to be able to load a byte array into the memory at a predefined address before starting the execution. There could be multiple data segments, each with its offset and size.

It looks like LLVM is predetermining where it's going to place all of the rodata, the main stack, and the start of the heap. They set the __data_end and __heap_base values as you'd expect; these are the index, in pages, from the start of linear memory, where the end of the rodata segment, and the bottom of the heap are, respectively. The stack grows down, so it's address is set (via $__stack_pointer) to the start of the rodata segment, since the stack consumes the first page. So basically, memory is laid out initially as follows:

[stack (64k/1 page) | rodata (64k/1 page) | heap (0k/pages)]
                    ^                     ^
                    |                    / \
            $__stack_pointer    __data_end  __heap_base

So this is basically the same type of thing I had intended to do with our linker, but LLVM nicely does some of this work for us already. I think we can plan to use globals for this too, but I may add a new variant to represent a section with a specific starting address and size, which we can then take into account when laying out the heap during codegen. The rodata in your example doesn't take up a full page, but they allocate a full page to it anyway, which is different than the default behavior of our globals, but that's easy to accommodate.

@bitwalker I implemented Wasm data section parsing up to the Module::declare_data_segment call introduced in #23. I'll add the actual call after #23 is merged, and I rebase on top of it.
Rust's static mut ends up in Wasm as .data (see test for this Rust code) so I'm deducting readonly property from the data segment name (.rodata substring means it's read-only).

@bitwalker I added a test with Rust memory allocations linking to std and got ctz and clz ops in the dlmalloc code and call_indirect in core::fmt and std::panicking. See - 4a9e303

@bitwalker I have not found any shr_s, div_s, or rem_s in the dlmalloc wasm code. I did find a couple of le_s and lt_s though.

@bitwalker I finished the house cleaning, and it's ready for review. Check TODO section in the description for what's left. I can continue working on this PR, or we can create issues for them, and I will tackle them in the new PRs.

@bitwalker I see #23 is merged. I'd like to rebase this branch on the new main branch. But I think the force push might mess with any comments you might have written as part of the review.

@greenhat Go ahead and rebase, I haven't had a chance to get my review completed yet, but can push without messing it up on my end

@greenhat Can you rebase this on the bitwalker/codegen branch? I'd like to merge both, and it'll make things simpler if this branch is based on the latest stuff in the miden-hir* crates. I'm going to do my review today regardless, but I'll hold off merging this until #27 is merged, basically treating them as a stacked set of changes.

@greenhat Can you rebase this on the bitwalker/codegen branch? I'd like to merge both, and it'll make things simpler if this branch is based on the latest stuff in the miden-hir* crates. I'm going to do my review today regardless, but I'll hold off merging this until #27 is merged, basically treating them as a stacked set of changes.

Will do!

I addressed all the notes.

Besides that, there are two issues:

• make an issue for fixing using popcnt for ctz, clz translation (used to unblock dlmalloc translation);
• dlmalloc compilation test is set to "ignored" because of the different mangled function names on CI. Figure out why the hash in mangled function names is so unstable, or even better - add an option to demangle function names and remove the hash, and use this option in tests.

I'd rather make issues and fix them in the next PR.

Awesome! I think we can get this merged now.

• make an issue for fixing using popcnt for ctz, clz translation (used to unblock dlmalloc translation);

Sure thing, I can take care of that ASAP

• dlmalloc compilation test is set to "ignored" because of the different mangled function names on CI. Figure out why the hash in mangled function names is so unstable, or even better - add an option to demangle function names and remove the hash, and use this option in tests.

I'm assuming the problem is we're trying to call a function that gets mangled. To do that, you have two options:

1. Add the #[no_mangle] attribute to an exported function that calls the desired function in the same compilation unit (i.e. if you are compiling a program that uses dlmalloc, and the function you want to call is exported from dlmalloc, you can add a function wrapper in the program that is no_mangle which calls the desired function), this will allow you to call it using an unmangled name
2. Search for the mangled symbol in the module, demangling each symbol as you go until you find the desired function.

Fundamentally, the mangling scheme is so unstable because it includes in it the hash of the build, i.e. the same hash you find in the target/debug/<crate>-<hash> folder name. In addition, symbol names are monomorphized, so they include instantiations of all generic parameters even when de-mangled, e.g. foo::bar::<T, U>::baz, so it's not always straightforward to find the symbol you want even when de-mangled. I've run into this before with another compiler project, where I needed to take over the responsibilities of the start function (i.e. the function which is called prior to main and which initializes the runtime), so I needed to invoke the Rust standard library function which initializes the Rust runtime. That function is not officially public API, but it is exported so that it can be called by Rust executables. I had to get creative to figure out what the symbol was and use it when compiling my runtime (if you're curious, this is the build script. We could do something similar if needed, but obviously that's a gross hack that we'd prefer not to do if we can help it.

Unfortunately, I don't think we can de-mangle function names when compiling to Miden, since they will almost certainly violate Miden's symbol naming rules. However, we could maybe do this for modules we want to test using the emulator, since in the IR we don't have any real restrictions on naming (aside from those imposed by the NamingConvention validation rule, which could be disabled in such cases).

• dlmalloc compilation test is set to "ignored" because of the different mangled function names on CI. Figure out why the hash in mangled function names is so unstable, or even better - add an option to demangle function names and remove the hash, and use this option in tests.

I'm assuming the problem is we're trying to call a function that gets mangled. To do that, you have two options:

Oh no, it's much simpler than that. I don't need to call the function. The problem is that expect! test for dlmalloc that checks the produced wat file fails on CI because hashes in mangled function names are different from the "golden"/expected ones that I built on my machine.

1. Add the #[no_mangle] attribute to an exported function that calls the desired function in the same compilation unit (i.e. if you are compiling a program that uses dlmalloc, and the function you want to call is exported from dlmalloc, you can add a function wrapper in the program that is no_mangle which calls the desired function), this will allow you to call it using an unmangled name
2. Search for the mangled symbol in the module, demangling each symbol as you go until you find the desired function.

Fundamentally, the mangling scheme is so unstable because it includes in it the hash of the build, i.e. the same hash you find in the target/debug/<crate>-<hash> folder name. In addition, symbol names are monomorphized, so they include instantiations of all generic parameters even when de-mangled, e.g. foo::bar::<T, U>::baz, so it's not always straightforward to find the symbol you want even when de-mangled. I've run into this before with another compiler project, where I needed to take over the responsibilities of the start function (i.e. the function which is called prior to main and which initializes the runtime), so I needed to invoke the Rust standard library function which initializes the Rust runtime. That function is not officially public API, but it is exported so that it can be called by Rust executables. I had to get creative to figure out what the symbol was and use it when compiling my runtime (if you're curious, this is the build script. We could do something similar if needed, but obviously that's a gross hack that we'd prefer not to do if we can help it.

Cool hack!

Unfortunately, I don't think we can de-mangle function names when compiling to Miden, since they will almost certainly violate Miden's symbol naming rules. However, we could maybe do this for modules we want to test using the emulator, since in the IR we don't have any real restrictions on naming (aside from those imposed by the NamingConvention validation rule, which could be disabled in such cases).

I agree, demangled names will violate Miden's symbol naming rules. I'm thinking about cutting out the hash from the mangled name. It should be hidden behind the option.

EDIT: I created an issue for this - #41